Base classes for accelerator refactoring #5715

justusschock · 2021-01-30T14:19:49Z

What does this PR do?

Adds the base classes from #5616

Before submitting

Was this discussed/approved via a GitHub issue? (not for typos and docs)
Did you read the contributor guideline, Pull Request section?
Did you make sure your PR does only one thing, instead of bundling different changes together?
Did you make sure to update the documentation with your changes? (if necessary)
Did you write any new necessary tests? (not for typos and docs)
Did you verify new and existing tests pass locally with your changes?
Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

Is this pull request ready for review? (if not, please submit in draft mode)
Check that all items from Before submitting are resolved
Make sure the title is self-explanatory and the description concisely explains the PR
Add labels and milestones (and optionally projects) to the PR so it can be classified
Check that target branch and milestone match!

Did you have fun?

Make sure you had fun coding 🙃

Co-Authored with @awaelchli

Co-Authored with @awaelchi

Co-authored with @awaelchi

Co-Authored with @awaelchi

Co-authored with @awaelchi

pep8speaks · 2021-01-30T14:19:52Z

Hello @justusschock! Thanks for updating this PR.

In the file pytorch_lightning/accelerators/accelerator.py:

Line 261:121: E501 line too long (121 > 120 characters)
Line 375:26: W292 no newline at end of file

In the file pytorch_lightning/plugins/base_plugin.py:

Line 57:14: W292 no newline at end of file

In the file pytorch_lightning/plugins/precision/__init__.py:

Line 1:1: W391 blank line at end of file

Comment last updated at 2021-01-30 19:15:52 UTC

Co-authored-by: @awaelchi

awaelchli

looking forward to it

SeanNaren

Looks good!

SeanNaren · 2021-01-30T15:35:12Z

pytorch_lightning/accelerators/accelerator.py

+        with self.precision_plugin.val_step_context():
+            with self.training_type_plugin.val_step_context():
+                return self.lightning_module.validation_step(*args)


This type of logic makes me think the precision plugin should live in the training type plugin, and that the base training type plugin manage the precision logic

Let's merge it as is and then change it later on :)

agreed with @SeanNaren - that cleans up the interface for the accelerator then. conceptually the accelerator simply manages the training plugin with additional lifecycle hooks, and the training plugin is responsible for precision/whatever else(?)

Yep, I also agree with @SeanNaren and there are plans to support that on top of the existing PoC since then this is probably easier to change then now.

Borda · 2021-01-30T18:48:58Z

replacing usage of all unexisting Plugins and annotating with fixme

Borda

for all these @abstractmethod with raise NotImplementedError use only one...

@abstractmethod cannot be instantiated till all these methods are implemented
raise NotImplementedError can be instantiated, but will crash when touching one of these

pytorch_lightning/accelerators/accelerator.py

pytorch_lightning/plugins/precision/precision_plugin.py

pytorch_lightning/plugins/training_type/training_type_plugin.py

codecov · 2021-01-30T19:07:22Z

Codecov Report

Merging #5715 (1c8771a) into release/1.2-dev (21d313e) will decrease coverage by 1%.
The diff coverage is 100%.

@@               Coverage Diff                @@
##           release/1.2-dev   #5715    +/-   ##
================================================
- Coverage               89%     88%    -1%     
================================================
  Files                  168     169     +1     
  Lines                12423   12673   +250     
================================================
+ Hits                 11102   11180    +78     
- Misses                1321    1493   +172

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>

ananthsub

@justusschock @awaelchli @SeanNaren what's the expected control flow across trainer/training loop/accelerator/training type plugin? i'm getting mixed up in trying to figure out what will call what when

ananthsub · 2021-01-30T19:45:39Z

pytorch_lightning/accelerators/accelerator.py

+        with self.precision_plugin.val_step_context():
+            with self.training_type_plugin.val_step_context():
+                return self.lightning_module.validation_step(*args)


agreed with @SeanNaren - that cleans up the interface for the accelerator then. conceptually the accelerator simply manages the training plugin with additional lifecycle hooks, and the training plugin is responsible for precision/whatever else(?)

pytorch_lightning/plugins/precision/precision_plugin.py

ananthsub · 2021-01-30T19:55:42Z

pytorch_lightning/plugins/base_plugin.py

+import torch
+
+
+class Plugin(object):


are all of these needed for all plugins? are there any steps here that we could/should move to the training type plugin?

pytorch_lightning/plugins/training_type/training_type_plugin.py

Borda · 2021-01-30T20:00:34Z

@ananthsub good comments let address them in follow-up PRs :] #5718

ananthsub · 2021-01-31T02:30:23Z

pytorch_lightning/plugins/training_type/training_type_plugin.py

+
+
+class TrainingTypePlugin(Plugin, ABC):
+    """A Plugin to change the behaviour of the training, validation and test-loop."""


@justusschock - another question: currently these training loop functions are set on the accelerator backend in fit() - should these be represented in the interface here or on the accelerator?
What do you think about the training type plugin accepting these as constructor arguments, assuming we can define some base interfaces for train/evaluation/test loop?

I don't think this is needed anymore, since we pass the Trainer to start_training etc, where we can directly call trainer.train

justusschock added 5 commits January 30, 2021 14:51

add basic accelerator class.

195ee32

Co-Authored with @awaelchi

Add base plugin class.

b449f5b

Co-authored with @awaelchi

add basic trainign type plugin.

a02e7d5

Co-Authored with @awaelchi

add basic precision plugin.

f7adc77

Co-Authored with @awaelchi

Add missing inits.

a503631

Co-authored with @awaelchi

justusschock added the refactor label Jan 30, 2021

justusschock requested review from ananthsub, awaelchli, Borda and tchaton January 30, 2021 14:19

justusschock self-assigned this Jan 30, 2021

justusschock requested review from SeanNaren and williamFalcon as code owners January 30, 2021 14:19

justusschock added this to the 1.2 milestone Jan 30, 2021

pep8

0c728f3

Co-authored-by: @awaelchi

awaelchli approved these changes Jan 30, 2021

View reviewed changes

SeanNaren approved these changes Jan 30, 2021

View reviewed changes

SeanNaren reviewed Jan 30, 2021

View reviewed changes

awaelchli added 3 commits January 30, 2021 16:49

ignore flake8

e700890

coverage omit

ce2ce0c

imports in init

a341186

awaelchli force-pushed the ref/base_classes branch from e940e5e to a341186 Compare January 30, 2021 16:29

This was referenced Jan 30, 2021

Accelerator Refactor: Precision Plugins #5718

Merged

Hardware specific parts of Accelerator Refactoring #5719

Merged

accelerator refactor - add parallel plugins #5714

Merged

lost

1bd6a14

Borda added 4 commits January 30, 2021 19:52

imports

6930a1e

flake8

e58532e

.

77b1b28

.

ec8fc9a

Borda approved these changes Jan 30, 2021

View reviewed changes

chlog

9b65220

justusschock and others added 7 commits January 30, 2021 20:09

Update pytorch_lightning/plugins/training_type/training_type_plugin.py

187afa9

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>

Update pytorch_lightning/plugins/training_type/training_type_plugin.py

6d0e153

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>

Update pytorch_lightning/plugins/training_type/training_type_plugin.py

0301c03

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>

Update pytorch_lightning/plugins/training_type/training_type_plugin.py

3e1b92d

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>

Update pytorch_lightning/plugins/training_type/training_type_plugin.py

eaa4147

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>

Update pytorch_lightning/plugins/training_type/training_type_plugin.py

e35f64c

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>

Update pytorch_lightning/plugins/training_type/training_type_plugin.py

1c8771a

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>

Borda enabled auto-merge (squash) January 30, 2021 19:38

Borda merged commit 5d239cc into release/1.2-dev Jan 30, 2021

Borda deleted the ref/base_classes branch January 30, 2021 19:55

ananthsub reviewed Jan 30, 2021

View reviewed changes

ananthsub reviewed Jan 31, 2021

View reviewed changes

Borda added the ready PRs ready to be merged label Jan 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Base classes for accelerator refactoring #5715

Base classes for accelerator refactoring #5715

justusschock commented Jan 30, 2021 •

edited by Borda

Loading

pep8speaks commented Jan 30, 2021 •

edited

Loading

awaelchli left a comment

SeanNaren left a comment

SeanNaren Jan 30, 2021

justusschock Jan 30, 2021

ananthsub Jan 30, 2021

justusschock Jan 30, 2021

Borda commented Jan 30, 2021

Borda left a comment

codecov bot commented Jan 30, 2021 •

edited

Loading

ananthsub left a comment

ananthsub Jan 30, 2021

ananthsub Jan 30, 2021

Borda commented Jan 30, 2021 •

edited

Loading

ananthsub Jan 31, 2021

justusschock Jan 31, 2021



		class TrainingTypePlugin(Plugin, ABC):
		"""A Plugin to change the behaviour of the training, validation and test-loop."""

Base classes for accelerator refactoring #5715

Base classes for accelerator refactoring #5715

Conversation

justusschock commented Jan 30, 2021 • edited by Borda Loading

What does this PR do?

Before submitting

PR review

Did you have fun?

pep8speaks commented Jan 30, 2021 • edited Loading

Comment last updated at 2021-01-30 19:15:52 UTC

awaelchli left a comment

Choose a reason for hiding this comment

SeanNaren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Borda commented Jan 30, 2021

Borda left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 30, 2021 • edited Loading

Codecov Report

ananthsub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Borda commented Jan 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justusschock commented Jan 30, 2021 •

edited by Borda

Loading

pep8speaks commented Jan 30, 2021 •

edited

Loading

codecov bot commented Jan 30, 2021 •

edited

Loading

Borda commented Jan 30, 2021 •

edited

Loading